IX: MultiImp Implementation #2779

ccorbo · 2023-05-15T19:05:23Z

These changes implements multiimp functionality
Also bidder param sid is added and passed through

SyntaxNode

I have concerns over the feature concept. It looks like the server is picking up on a signal from a request to enable a feature for a set amount of time. It's bad practice for the server to keep state in this manner. Why is this required versus looking for a feature flag on each request?

SyntaxNode · 2023-05-23T18:09:16Z

adapters/ix/ix.go

@@ -18,46 +19,69 @@ import (
 	"github.com/prebid/openrtb/v19/openrtb2"
 )

+const (
+	FtHandleMultiImpOnSingleRequest = "pbs_handle_multi_imp_on_single_req"


Nitpicks: There is no need to export this constant (or ExpireFtTime) so please change them to starting with a lowercase letter.

SyntaxNode · 2023-05-23T18:09:52Z

adapters/ix/ix.go

+	FtHandleMultiImpOnSingleRequest = "pbs_handle_multi_imp_on_single_req"
+)
+
+const ExpireFtTime = 3600 //1 hour


Nitpick: Calling this expireFtTimeSeconds might help provide more context in addition to the comment. However, please see my other comments regarding the server keeping this kind of state.

SyntaxNode · 2023-05-23T18:11:45Z

adapters/ix/ix.go

 	// Multi-size banner imps are split into single-size requests.
 	// The first size imp requests are added to the first slice.
 	// Additional size requests are added to the second slice and are merged with the first at the end.
 	// Preallocate the max possible size to avoid reallocating arrays.
 	requests := make([]*adapters.RequestData, 0, a.maxRequests)
 	multiSizeRequests := make([]*adapters.RequestData, 0, a.maxRequests-nImp)
+	errs := make([]error, 0, 1)


Let's optimize for the average case where there is no error. If you use var errs []error there will not be any allocations unless an error is encountered. That will reduce GC pressure a little bit. Same applies to handleMultiImpInSingleRequest.

SyntaxNode · 2023-05-23T18:16:09Z

adapters/ix/ix.go

+		}
+
+		if err := moveSid(&imp); err != nil {
+			errs = append(errs, err)


To Confirm: It is your intent to continue with the auction if moveSid fails? Not sure if omitting the continue statement here is intentional or an oversight.

Confirming it is intentional to still make the bidrequest if that function errors out

SyntaxNode · 2023-05-23T18:18:58Z

adapters/ix/ix.go

+		if len(siteIds) == 1 {
+			for siteId := range siteIds {
+				site.Publisher.ID = siteId
+			}


This is looping through and setting the publisher id each time, such that the last siteId in the array always wins. You can avoid the loop entirely and access the last item in the slice directly.

SyntaxNode · 2023-05-23T18:23:36Z

adapters/ix/ix.go

+
+// moves sid from imp[].ext.bidder.sid to imp[].ext.sid
+func moveSid(imp *openrtb2.Imp) error {
+	if imp.Ext == nil || len(imp.Ext) == 0 {


Nitpick: This can just be if len(imp.Ext) == 0 { since len gracefully handles the nil case.

SyntaxNode · 2023-05-23T18:25:15Z

adapters/ix/ix.go

+}
+
+func setFeatureToggles(a *IxAdapter, ext *json.RawMessage) {
+	if *ext == nil {


Should this be if ext == nil? You want to verify the ext has a non-nil pointer before dereferencing. If this is indeed a mistake, consider adding test coverage to catch it.

SyntaxNode · 2023-05-23T18:26:02Z

adapters/ix/ix.go

+	}
+
+	var f interface{}
+	err := json.Unmarshal(*ext, &f)


This is a very expensive operation. Is there a reason you can't use a defined structure with unmarshal?

SyntaxNode · 2023-05-23T18:32:18Z

adapters/ix/ix.go

+		if ft, ok := features.(map[string]interface{}); ok {
+			for k, v := range ft {
+				if activated, ok := v.(map[string]interface{})["activated"]; ok {
+					a.clientFeatureStatusMap[k] = FeatureTimestamp{


This is not thread safe and will inevitably lead to a panic or unexpected behavior. To fix this, you will need to protect read/write access with a lock of some kind.

I'm confused as to why the server needs to keep track of a feature from an incoming request? Why is each request not evaluated on it's own merit?

Further, Prebid Server is hosted in many large clusters. Such that encountering a single request with a feature enablement will only affect 1 out of many servers.

SyntaxNode · 2023-05-23T18:35:43Z

adapters/ix/ix_test.go

@@ -276,3 +294,220 @@ func TestMakeRequestsErrIxDiag(t *testing.T) {
 	_, errs := bidder.MakeRequests(req, nil)
 	assert.Len(t, errs, 1)
 }
+
+func TestSetFeatureToggles(t *testing.T) {


It's fine if you want to add unit tests, but we request code coverage via the json framework as that performs additional checks such as for memory corruption issues. We only consider TestJsonSamples when evaluating code coverage.

ccorbo · 2023-05-26T18:30:33Z

I have concerns over the feature concept. It looks like the server is picking up on a signal from a request to enable a feature for a set amount of time. It's bad practice for the server to keep state in this manner. Why is this required versus looking for a feature flag on each request?

Hey Scott - First thanks for the review!
This feature toggle concept here is to control whether we are sending single-imp requests versus multi-imp requests via a signal from the exchange.
For example:
On request 0 -> FT State is off -> Send single imp requests
Response 0 -> Signals the adapter to send multiimp requests.
On all subsequent requests while the feature toggle is active:
Request n -> FT State is on -> Send multiimp requests

That is the reason we need to keep state in the adapter, to control subsequent requests.

Let me know what you think of this. Will remove the features controlled from the exchange concept and discuss with my team if we want to avoid the server keeping state.

Thank you!

gargcreation1992 · 2023-06-08T09:59:18Z

I have concerns over the feature concept. It looks like the server is picking up on a signal from a request to enable a feature for a set amount of time. It's bad practice for the server to keep state in this manner. Why is this required versus looking for a feature flag on each request?

Hey Scott - First thanks for the review! This feature toggle concept here is to control whether we are sending single-imp requests versus multi-imp requests via a signal from the exchange. For example: On request 0 -> FT State is off -> Send single imp requests Response 0 -> Signals the adapter to send multiimp requests. On all subsequent requests while the feature toggle is active: Request n -> FT State is on -> Send multiimp requests

That is the reason we need to keep state in the adapter, to control subsequent requests.

Let me know what you think of this. Will remove the features controlled from the exchange concept and discuss with my team if we want to avoid the server keeping state.

Thank you!

@ccorbo If I understand this feature, please do not maintain any kind of state in adapter code. If you want to write any business logic then please code that logic in your bidder endpoint.

ccorbo · 2023-06-14T14:09:18Z

Removed feature toggle logic

gargcreation1992 · 2023-06-21T09:55:45Z

Removed feature toggle logic

@ccorbo Could you please help in explaining what you are trying to accomplish with this PR?

ccorbo · 2023-06-21T13:47:33Z

Removed feature toggle logic

@ccorbo Could you please help in explaining what you are trying to accomplish with this PR?

Hi - This PR changes how we handle multi-imp requests. Previously multi-imp requests would be flattened into multiple requests, with one request for each Imp.

Now there will be a single request, with one request for multi-imp.

Also this changes sid to be signaled at imp.ext.sid

oronno · 2023-06-22T14:50:11Z

I have updated this PR with changes to handle banner multi-size. I have also removed the maximum impression limit and added proper test cases that handle multi-imp, banner multi-size, and multiple site IDs.

In summary, this PR as a whole will accomplish the following:

Removed the flattening logic for multi-impression and multi-size banners. As a result, the IX Adapter will now send a single request to the Exchange for multi-impression or multi-size banner requests.
Removed the maximum impression limit.
Updated the sid location by moving sid from imp[].ext.bidder.sid to imp[].ext.sid.
Added test cases to cover these behaviors.

CC: @ccorbo @gargcreation1992 @SyntaxNode

…h from bidder params

bsardo · 2023-06-28T17:19:05Z

Hi @ccorbo, if you don't mind, can you merge with master instead of force pushing going forward? It saves us some time when reviewing. Thanks!

ccorbo · 2023-06-28T20:09:12Z

Hi @ccorbo, if you don't mind, can you merge with master instead of force pushing going forward? It saves us some time when reviewing. Thanks!

Yep will do - sorry about that!

ccorbo · 2023-06-29T19:20:58Z

@bsardo Hi Brian - Is it possible to get a review on this before next Wednesdays release? Thanks in advance

gargcreation1992 · 2023-07-03T10:41:36Z

adapters/ix/ix.go

+
+	for _, imp := range requestCopy.Imp {
+		var err error
+		if err = parseSiteId(&imp, uniqueSiteIDs); err != nil {


In parseSiteId function, you are unmarshalling the impression json into var of type openrtb_ext.ExtImpIx and doing the same thing again in moveSid function. Requesting you to please take out the common umarshal logic from both parseSiteId and moveSid function to improve on efficiency.

gargcreation1992 · 2023-07-03T10:57:53Z

adapters/ix/ix_test.go

@@ -276,3 +274,63 @@ func TestMakeRequestsErrIxDiag(t *testing.T) {
 	_, errs := bidder.MakeRequests(req, nil)
 	assert.Len(t, errs, 1)
 }
+
+func TestMoveSid(t *testing.T) {


Requesting you to please move these test case to use json framework as this is the recommended way to test a bid adapter code.

gargcreation1992 · 2023-07-04T06:21:29Z

adapters/ix/ixtest/supplemental/sid.json

+                  "siteId": "569749",
+                  "sid": "1234"
+                },
+                "sid": "1234"


Is it necessary to move sid under ext if that value is already present in ext.bidder.sid? If your bidder uses ext.sid then could you please change it to use ext.bidder.sid instead?

@gargcreation1992 Yes it is necessary in the current state to move to ext.sid. We can revisit in the future, but its something we'd like to move for now and get it out

bretg · 2023-07-05T18:24:18Z

@ccorbo - do you plan to port this to PBS-Java or shall we just put it on the list?

ccorbo · 2023-07-05T19:02:18Z

@bretg - Can you please put it on the list, thanks in advance

Co-authored-by: Chris Corbo <chris.corbo@indexexchange.com> Co-authored-by: Oronno Mamun <oronno.mamun@indexexchange.com>

@akkapur

* Fix: deal tiers no longer ignored due to presence of tid (prebid#2829) * CAPT-787: GPP support for imds bidder. (prebid#2867) Co-authored-by: Timothy M. Ace <tace@imds.tv> * Adsinteractive: change usersync endpoint to https (prebid#2861) Co-authored-by: Balint Vargha <varghabalint@gmail.com> * consumable adapter: add gpp support (prebid#2883) * feat: IX Bid Adapter - gpp support for user sync urls (prebid#2873) Co-authored-by: Chris Corbo <chris.corbo@indexexchange.com> * fix: update links in readme (prebid#2888) authored by @akkapur * New Adapter: AIDEM (prebid#2824) Co-authored-by: AndreaC <67786179+darkstarac@users.noreply.github.com> Co-authored-by: Andrea Tumbarello <Wazabit@users.noreply.github.com> Co-authored-by: darkstar <canazza@wazabit.it> * Improve Digital adapter: Set currency in bid response (prebid#2886) * Sharethrough: Support multiformat bid request impression (prebid#2866) * Triplelift Bid Adapter: Adding GPP Support (prebid#2887) * YahooAdvertising rebranding to Yahoo Ads. (prebid#2872) Co-authored-by: oath-jac <dsp-supply-prebid@verizonmedia.com> * IX: MultiImp Implementation (prebid#2779) Co-authored-by: Chris Corbo <chris.corbo@indexexchange.com> Co-authored-by: Oronno Mamun <oronno.mamun@indexexchange.com> * Exchange unit test fix (prebid#2868) * Semgrep rules for adapters (prebid#2833) * IX: Remove glog statement (prebid#2909) * Activities framework (prebid#2844) * PWBID: Update Default Endpoint (prebid#2903) * script to run semgrep tests against adapter PRs (prebid#2907) authored by @onkarvhanumante * semgrep rule to detect undesirable package imports in adapter code (prebid#2911) * update package-import message (prebid#2913) authored by @onkarvhanumante * Bump google.golang.org/grpc from 1.46.2 to 1.53.0 (prebid#2905) --------- Co-authored-by: Brian Sardo <1168933+bsardo@users.noreply.github.com> Co-authored-by: Timothy Ace <github.com-1@timothyace.com> Co-authored-by: Timothy M. Ace <tace@imds.tv> Co-authored-by: balintvargha <122350182+balintvargha@users.noreply.github.com> Co-authored-by: Balint Vargha <varghabalint@gmail.com> Co-authored-by: Jason Piros <jasonpiros@gmail.com> Co-authored-by: ccorbo <ccorbo2013@gmail.com> Co-authored-by: Chris Corbo <chris.corbo@indexexchange.com> Co-authored-by: Ankush <ankush.kapur@gmail.com> Co-authored-by: Giovanni Sollazzo <gs@aidem.com> Co-authored-by: AndreaC <67786179+darkstarac@users.noreply.github.com> Co-authored-by: Andrea Tumbarello <Wazabit@users.noreply.github.com> Co-authored-by: darkstar <canazza@wazabit.it> Co-authored-by: Jozef Bartek <31618107+jbartek25@users.noreply.github.com> Co-authored-by: Max Dupuis <118775839+maxime-dupuis@users.noreply.github.com> Co-authored-by: Patrick Loughrey <ploughrey@triplelift.com> Co-authored-by: radubarbos <raduiquest79@gmail.com> Co-authored-by: oath-jac <dsp-supply-prebid@verizonmedia.com> Co-authored-by: Oronno Mamun <oronno.mamun@indexexchange.com> Co-authored-by: Veronika Solovei <kalypsonika@gmail.com> Co-authored-by: Onkar Hanumante <onkar.hanumante@xandr.com> Co-authored-by: Stephen Johnston <tiquortoo@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

ccorbo force-pushed the multiimp_PB-1382 branch from 0b2d7e1 to b4809de Compare May 15, 2023 19:18

gargcreation1992 assigned gargcreation1992 and Sonali-More-Xandr May 16, 2023

SyntaxNode reviewed May 23, 2023

View reviewed changes

ccorbo force-pushed the multiimp_PB-1382 branch from 74d9ed5 to b30e3d5 Compare June 12, 2023 13:35

Chris Corbo added 5 commits June 22, 2023 11:04

feat: multiimp work in ixbidadapter behind ft, pass sid signal throug…

d563ed2

…h from bidder params

chore: pr feedback

d70d9d5

chore: update siteid logic

136fc5a

chore: remove feature toggle logic

91e8e76

chore: small refactor

6ffa3b6

ccorbo force-pushed the multiimp_PB-1382 branch from 2604332 to 6ffa3b6 Compare June 22, 2023 15:04

feat: multi-size handle and set siteId properly

f9908c7

bsardo changed the title ~~IX Bid Adapter: MultiImp Implementation~~ IX: MultiImp Implementation Jun 28, 2023

gargcreation1992 reviewed Jul 3, 2023

View reviewed changes

chore: pr feedback

7d7db60

gargcreation1992 reviewed Jul 4, 2023

View reviewed changes

gargcreation1992 approved these changes Jul 5, 2023

View reviewed changes

Sonali-More-Xandr approved these changes Jul 5, 2023

View reviewed changes

Sonali-More-Xandr merged commit c8ab53d into prebid:master Jul 5, 2023

cross-repo-issue-creation bot mentioned this pull request Jul 5, 2023

Port PR from PBS-Go: IX: MultiImp Implementation prebid/prebid-server-java#2484

Closed

Peiling-Ding pushed a commit to ParticleMedia/prebid-server that referenced this pull request Jul 6, 2023

IX: MultiImp Implementation (prebid#2779)

d2ae80b

Co-authored-by: Chris Corbo <chris.corbo@indexexchange.com> Co-authored-by: Oronno Mamun <oronno.mamun@indexexchange.com>

Peiling-Ding pushed a commit to ParticleMedia/prebid-server that referenced this pull request Jul 14, 2023

IX: MultiImp Implementation (prebid#2779)

72a0c28

Co-authored-by: Chris Corbo <chris.corbo@indexexchange.com> Co-authored-by: Oronno Mamun <oronno.mamun@indexexchange.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IX: MultiImp Implementation #2779

IX: MultiImp Implementation #2779

ccorbo commented May 15, 2023 •

edited

Loading

SyntaxNode left a comment

SyntaxNode May 23, 2023

SyntaxNode May 23, 2023

SyntaxNode May 23, 2023

SyntaxNode May 23, 2023

ccorbo Jun 7, 2023

SyntaxNode May 23, 2023

SyntaxNode May 23, 2023

SyntaxNode May 23, 2023

SyntaxNode May 23, 2023

SyntaxNode May 23, 2023

SyntaxNode May 23, 2023

ccorbo commented May 26, 2023

gargcreation1992 commented Jun 8, 2023

ccorbo commented Jun 14, 2023

gargcreation1992 commented Jun 21, 2023

ccorbo commented Jun 21, 2023

oronno commented Jun 22, 2023

bsardo commented Jun 28, 2023

ccorbo commented Jun 28, 2023

ccorbo commented Jun 29, 2023

gargcreation1992 Jul 3, 2023

gargcreation1992 Jul 3, 2023

gargcreation1992 Jul 4, 2023

ccorbo Jul 5, 2023

bretg commented Jul 5, 2023

ccorbo commented Jul 5, 2023

IX: MultiImp Implementation #2779

IX: MultiImp Implementation #2779

Conversation

ccorbo commented May 15, 2023 • edited Loading

SyntaxNode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccorbo commented May 26, 2023

gargcreation1992 commented Jun 8, 2023

ccorbo commented Jun 14, 2023

gargcreation1992 commented Jun 21, 2023

ccorbo commented Jun 21, 2023

oronno commented Jun 22, 2023

bsardo commented Jun 28, 2023

ccorbo commented Jun 28, 2023

ccorbo commented Jun 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bretg commented Jul 5, 2023

ccorbo commented Jul 5, 2023

ccorbo commented May 15, 2023 •

edited

Loading